Conversation
db6fc11 to
a9486d8
Compare
ee1ec9c to
7358837
Compare
There was a problem hiding this comment.
Pull request overview
This is a large-scale data layer rework that migrates from database-backed caching (BaseTimelineRemoteMediator, DbPagingTimelineWithStatus, StatusContent, UserContent) to a new architecture using RemoteLoader<T> / CacheableRemoteLoader<T> interfaces that work directly with UI model types (UiTimelineV2, UiProfile). The PR also introduces handler/loader abstractions for users, relations, notifications, posts, lists, and emojis, and updates all platform UI code (Android, iOS, Desktop) to use the new model types.
Changes:
- Replace
BaseTimelineRemoteMediator/BasePagingSourcewithRemoteLoader<T>/CacheableRemoteLoader<T>interfaces across all data sources (Mastodon, Misskey, Bluesky, VVO, RSS, guest) - Introduce new handler/loader/datasource abstractions (
UserHandler,RelationHandler,PostHandler,EmojiHandler,ListHandler, etc.) and refactor DB models (DbUser,DbStatus,EmojiContent) to store UI model types directly - Update all platform UI code (Swift/iOS, Compose/Android, Desktop) to use
UiTimelineV2,UiHandle, and new relation/action patterns
Reviewed changes
Copilot reviewed 235 out of 375 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
*/microblog/paging/RemoteLoader.kt |
New RemoteLoader<T> interface and toPagingSource() adapter |
*/microblog/paging/CacheableRemoteLoader.kt |
New CacheableRemoteLoader<T> interface with pagingKey |
*/microblog/paging/TimelineRemoteMediator.kt |
Reworked to wrap CacheableRemoteLoader<UiTimelineV2> |
*/vvo/*.kt, */misskey/*.kt, */mastodon/*.kt, */bluesky/*.kt |
Migrated all remote mediators/paging sources to new interfaces |
*/microblog/handler/*.kt |
New handler classes for users, relations, notifications, emoji, lists |
*/microblog/loader/*.kt |
New loader interfaces |
*/microblog/datasource/*.kt |
New datasource interfaces |
*/database/cache/model/*.kt |
DB models updated to store UiProfile/UiTimelineV2 directly |
*/database/cache/dao/*.kt |
DAO updates for new schema |
*/database/adapter/AccountTypeConverter.kt |
Type converters for new UI model types |
*/database/cache/mapper/User.kt |
New toDbUser() and upsertUsers() helpers |
iosApp/**/*.swift |
iOS UI updates for new model types |
app/**/*.kt, desktopApp/**/*.kt |
Android/Desktop UI updates |
compose-ui/**/*.kt |
Shared Compose UI updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| prevKey = null, | ||
| nextKey = nextPage.takeIf { it != params.key && data.any() }, | ||
| endOfPaginationReached = data.isEmpty(), | ||
| nextKey = (page + 1).toString(), |
There was a problem hiding this comment.
When endOfPaginationReached is true (i.e., data.isEmpty()), nextKey is still set to the next page number. This means the paging framework may attempt to load the next page even though the end has been reached. Previously the code used takeIf to return null when there was no more data. The nextKey should be null when end of pagination is reached.
| nextKey = (page + 1).toString(), | |
| nextKey = (page + 1).takeIf { data.isNotEmpty() }?.toString(), |
| when (request) { | ||
| PagingRequest.Refresh -> 0 | ||
| is PagingRequest.Append -> request.nextKey.toIntOrNull() ?: 0 | ||
| is PagingRequest.Prepend -> 0 |
There was a problem hiding this comment.
The PagingRequest.Prepend branch on line 25 is unreachable because prepend is already handled and returned early on line 18-19. This dead code is confusing; the when branch for Prepend should be removed here since it's already handled above.
| is PagingRequest.Prepend -> 0 | |
| else -> 0 |
| return PagingResult( | ||
| data = emptyList(), | ||
| nextKey = null, | ||
| previousKey = null, |
There was a problem hiding this comment.
Unlike other loaders in this PR that return PagingResult(endOfPaginationReached = true) for non-refresh requests, this loader returns a result with data = emptyList() without setting endOfPaginationReached. This inconsistency may cause unexpected behavior where the paging framework doesn't know pagination has ended. Use PagingResult(endOfPaginationReached = true) to match the pattern used elsewhere.
| previousKey = null, | |
| previousKey = null, | |
| endOfPaginationReached = true, |
| // override fun react( | ||
| // statusKey: MicroBlogKey, | ||
| // hasReacted: Boolean, | ||
| // reaction: String, | ||
| // ) { | ||
| // } |
There was a problem hiding this comment.
This commented-out code should either be removed entirely or replaced with a TODO comment explaining why the react method is not yet implemented for Pleroma. Leaving dead commented-out code reduces readability.
| // override fun react( | |
| // statusKey: MicroBlogKey, | |
| // hasReacted: Boolean, | |
| // reaction: String, | |
| // ) { | |
| // } | |
| // TODO: Implement ReactionDataSource.react for Pleroma when reactions are supported or required. |
| internal interface ReactionDataSource : AuthenticatedMicroblogDataSource { | ||
| fun react( | ||
| statusKey: MicroBlogKey, | ||
| hasReacted: Boolean, | ||
| reaction: String, | ||
| ) | ||
|
|
||
| fun emoji(): Cacheable<ImmutableMap<String, ImmutableList<UiEmoji>>> | ||
| // fun react( | ||
| // statusKey: MicroBlogKey, | ||
| // hasReacted: Boolean, | ||
| // reaction: String, | ||
| // ) | ||
| } |
There was a problem hiding this comment.
ReactionDataSource now extends AuthenticatedMicroblogDataSource but has no methods of its own (everything is commented out). This makes the interface a no-op marker interface. Consider removing it or adding a TODO with a plan for when the react method will be restored.
| val content = item.status.content | ||
| if (content is UiTimeline.ItemContent.Status) { | ||
| val content = item.status as? UiTimelineV2.Post | ||
| if (content is UiTimelineV2.Post) { |
There was a problem hiding this comment.
The as? safe cast on line 567 already ensures content is UiTimelineV2.Post or null. The subsequent is check on line 568 is redundant. Use a simple if (content != null) or combine into a single let block.
| if (content is UiTimelineV2.Post) { | |
| if (content != null) { |
| data = users.map { it.render(accountKey = accountKey) }, | ||
| prevKey = null, | ||
| nextKey = if (users.isEmpty()) null else params.key?.plus(1), | ||
| nextKey = if (users.isEmpty()) null else ((page ?: 0) + 1).toString(), |
There was a problem hiding this comment.
The endOfPaginationReached field is not explicitly set in this PagingResult. When users is not empty, the default value for endOfPaginationReached may be true, which would conflict with providing a non-null nextKey. Verify that the PagingResult default for endOfPaginationReached is false, or explicitly set it to users.isEmpty() for clarity and correctness.
| nextKey = if (users.isEmpty()) null else ((page ?: 0) + 1).toString(), | |
| nextKey = if (users.isEmpty()) null else ((page ?: 0) + 1).toString(), | |
| endOfPaginationReached = users.isEmpty(), |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7358837802
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -69,15 +72,15 @@ public class ProfilePresenter( | |||
| } else { | |||
| throw NoActiveAccountException | |||
| } | |||
| service.userById(userId).toUi() | |||
| (service as UserDataSource).userHandler.userById(userId).toUi() | |||
There was a problem hiding this comment.
Avoid casting guest data sources to UserDataSource
This cast breaks profile loading in guest mode: accountServiceFlow(AccountType.Guest, ...) returns GuestMastodonDataSource, which does not implement UserDataSource, so opening a profile as a guest now throws at runtime and the presenter falls into an error state instead of loading user data. Please gate this path by interface support (or provide a guest-compatible user loader) before calling userHandler.
Useful? React with 👍 / 👎.
| accountServiceFlow( | ||
| accountType = AccountType.Specific(event.accountKey), | ||
| repository = accountRepository, | ||
| ).collect { service -> |
There was a problem hiding this comment.
Consume deeplink event flow only once
accountServiceFlow(...) is a long-lived stream, so using collect here prevents the coroutine from completing and pendingUrl = null is never reached for flare-event:// links. That leaves this handler active indefinitely and prevents identical event URLs from being handled again because the pending state is never cleared. Use first/firstOrNull (or otherwise cancel after one emission) when processing a single deeplink event.
Useful? React with 👍 / 👎.
No description provided.